Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: create go.mod for x/params #17776

Merged
merged 5 commits into from
Oct 3, 2023
Merged

Conversation

odeke-em
Copy link
Collaborator

@odeke-em odeke-em commented Sep 17, 2023

Makes x/params/keeper a standalone Go module accessible via vanity URL path cosmossdk.io/x/params

Updates #11899

@odeke-em odeke-em marked this pull request as ready for review September 17, 2023 22:36
@odeke-em odeke-em requested a review from a team as a code owner September 17, 2023 22:36
@github-prbot github-prbot requested review from a team, facundomedica and julienrbrt and removed request for a team September 17, 2023 22:36
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why only the keeper? And we may want to do this before (#17360), so that the SDK does not import params anymore (which will give cyclic dep if not).

@odeke-em
Copy link
Collaborator Author

@julienrbrt I was doing one module at a time as advised in the original issue.

@julienrbrt
Copy link
Member

julienrbrt commented Sep 17, 2023

@julienrbrt I was doing one module at a time as advised in the original issue.

I see, but it should be the whole x/params to be its own go.mod, not only its keeper.

@odeke-em
Copy link
Collaborator Author

Gotcha! Indeed I had started with x/params but saw that it was deprecated and the notice in the README.md said each module housed its own params, let me make that update. Thanks @julienrbrt!

@odeke-em odeke-em force-pushed the go.mod-spin-out-x-params-keeper branch from 7c2443d to 937c7a3 Compare September 17, 2023 22:53
@odeke-em odeke-em changed the title refactor: create go.mod for x/params/keeper refactor: create go.mod for x/params Sep 17, 2023
@odeke-em odeke-em force-pushed the go.mod-spin-out-x-params-keeper branch from 937c7a3 to e4006c3 Compare September 18, 2023 00:53
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

A go mod tidy all is prob necessary.

x/params/keeper/README.md Outdated Show resolved Hide resolved
go.work.example Outdated
@@ -25,5 +25,6 @@ use (
./x/circuit
./x/feegrant
./x/evidence
./x/params/keeper
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still applicable

UPGRADING.md Outdated Show resolved Hide resolved
@odeke-em odeke-em force-pushed the go.mod-spin-out-x-params-keeper branch from 3464185 to b3c1e10 Compare October 2, 2023 04:16
@odeke-em odeke-em requested a review from tac0turtle October 2, 2023 05:24
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall looks good, there were some changes in the go mods that i believe can be reverted

go.mod Outdated
@@ -1,6 +1,4 @@
go 1.21

toolchain go1.21.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how come this was removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My Go's go mod tidy did that, let me manually revert. Sorry about that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh all good, this forces the usage of go 1.12 from my knowledge. what settings do you have on your go mod?

@@ -39,7 +39,7 @@ import (
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
consensusparamtypes "github.com/cosmos/cosmos-sdk/x/consensus/types"
minttypes "github.com/cosmos/cosmos-sdk/x/mint/types"
paramtypes "github.com/cosmos/cosmos-sdk/x/params/types"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we should be able to drop this dep?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's under tests/integration, so it's fine. We still test params for x/tx amino support

tests/starship/tests/go.mod Outdated Show resolved Hide resolved
@@ -13,14 +13,14 @@ require (
cosmossdk.io/store v1.0.0-rc.0
github.com/cometbft/cometbft v0.38.0
github.com/cosmos/cosmos-proto v1.0.0-beta.3
github.com/cosmos/cosmos-sdk v0.46.0-beta2.0.20230713190037-6a0ab4fd167f
github.com/cosmos/cosmos-sdk v0.47.5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should push this to the latest version of the releases?

cc @julienrbrt

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should either use a pseudo version from main (v0.46.0-beta2.xxxx) or a fake version (#17776 (comment)) and replace the SDK.

Definitely not a released version on main.

Makes x/params a standalone Go module accessible
via vanity URL path cosmossdk.io/x/params

Updates #11899
@odeke-em odeke-em force-pushed the go.mod-spin-out-x-params-keeper branch from b3c1e10 to 30b6984 Compare October 3, 2023 08:40
@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 3, 2023

[Cosmos SDK - x/params] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@julienrbrt julienrbrt added this pull request to the merge queue Oct 3, 2023
Merged via the queue into main with commit 0692fdc Oct 3, 2023
58 of 59 checks passed
@julienrbrt julienrbrt deleted the go.mod-spin-out-x-params-keeper branch October 3, 2023 12:59
@odeke-em
Copy link
Collaborator Author

odeke-em commented Oct 3, 2023

Oh wow, thank you very much @julienrbrt for getting this over the line! Thank you very much for the reviews birth of you @tac0turtle and @julienrbrt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants